-
Notifications
You must be signed in to change notification settings - Fork 305
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(boxai-sidebar): BoxAISidebar header added #3698
feat(boxai-sidebar): BoxAISidebar header added #3698
Conversation
Because this is a public repo, we should not paste Jira tickets. Shouldn't the "expand" button be right aligned? |
@@ -15,3 +13,5 @@ export default { | |||
token: global.TOKEN, | |||
}, | |||
}; | |||
|
|||
export const basic = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason for moving this line below default export?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No exact reason, just looked at other tests
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just to reiterate, the default
export is usually on the bottom, so the basic
export should be moved above it
Looks like style of the header is not matching other headers. It should be bold, also not sure about font size |
@MateuszMamczarz could confirm, that for now BoxAISidebarHeader should only have this Expand button on the left, near the Box AI title |
src/elements/content-sidebar/stories/tests/BoxAISidebar-visual.stories.tsx
Show resolved
Hide resolved
@@ -15,3 +13,5 @@ export default { | |||
token: global.TOKEN, | |||
}, | |||
}; | |||
|
|||
export const basic = {}; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
92883db
to
819b904
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
b9f2347
to
23a7549
Compare
justify-content: left; | ||
|
||
.bcs-title { | ||
padding-right: $space-3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could probably use gap in this case?
@@ -117,6 +117,11 @@ const messages = defineMessages({ | |||
defaultMessage: 'Modify General Task', | |||
description: 'modal title for when editing an existing general task', | |||
}, | |||
expandBoxAI: { | |||
id: 'be.expandBoxAI', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should add be.contentSidebar
to the id prefix since we dont want to risk any conflicts. i.e. in case some other part of the repo has the same message id - we can't have duplicates
@@ -24,3 +24,12 @@ export const BoxAIInSidebar: StoryObj<typeof BoxAISidebar> = { | |||
expect(sidebar).toBeInTheDocument(); | |||
}, | |||
}; | |||
|
|||
export const ExpandButtonCheck: StoryObj<typeof BoxAISidebar> = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i assume this test will become a visual regression for the entire expanded view in Box AI right? so maybe we can name it like BoxAIWithExpandedView
@@ -15,3 +13,5 @@ export default { | |||
token: global.TOKEN, | |||
}, | |||
}; | |||
|
|||
export const basic = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just to reiterate, the default
export is usually on the bottom, so the basic
export should be moved above it
const expandButton = screen.getByRole('button', { name: 'Expand' }); | ||
await userEvent.click(expandButton); | ||
|
||
expect(mockOnExpandClick).toHaveBeenCalled(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there can be issues and unexpected behavior if we forget to clear/reset the mock calls between tests. i think we handle this in a global configuration but just in case, it's better to not use reuse the fn mock for the expect
so it's more intentional about what should happen
e.g. check for a separate prop (defined in the local scope) and/or explicitly add an afterEach
afterEach(() => {
jest.clearAllMocks();
});
// or
const onExpandClick = jest.fn();
renderComponent({ onExpandClick });
...
expect(onExpandClick).toHaveBeenCalled();
05cc563
to
f791408
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM apart from some minor feedback.
@@ -117,6 +117,11 @@ const messages = defineMessages({ | |||
defaultMessage: 'Modify General Task', | |||
description: 'modal title for when editing an existing general task', | |||
}, | |||
expandBoxAI: { | |||
id: 'be.contentSidebarExpandBoxAI', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we namespace this to be similar to the other messages? Maybe be.contentSidebar.boxAI.expand
?
expandBoxAI: { | ||
id: 'be.contentSidebarExpandBoxAI', | ||
description: 'Default message for Box AI expand button in sidebar header', | ||
defaultMessage: 'Expand', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor, but let's move this up a line to stay consistent with the other messages.
@@ -117,6 +117,11 @@ const messages = defineMessages({ | |||
defaultMessage: 'Modify General Task', | |||
description: 'modal title for when editing an existing general task', | |||
}, | |||
expandBoxAI: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
boxAISidebarExpand
seems accurate?
} | ||
|
||
function BoxAISidebar() { | ||
function BoxAISidebar({ onExpandClick }: BoxAISidebarProps) { | ||
const { formatMessage } = useIntl(); | ||
|
||
return ( | ||
<SidebarContent | ||
className={'bcs-BoxAISidebar'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor, but this can be a regular string: className="bcs-BoxAISidebar"
<IconButton | ||
onClick={onExpandClick} | ||
icon={ArrowsExpand} | ||
aria-label={formatMessage(sidebarMessages.expandBoxAI)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We typically alphabetize our props whenever possible.
Reviewer out of office, but all feedback appears to have been addressed.
6a2d980
to
97c08ca
Compare
6d67640
Description
Added header to BoxAISidebar, which contains Expand button.
Screenshots